chunked: define error for partial pulls not available #2118

giuseppe · 2024-09-30T08:08:06Z

define a new error type so that the caller can determine whether it is safe to ignore the error and retrieve the resource fully.

Closes: #2115

the relative patch for containers/image is something like:

diff --git a/copy/single.go b/copy/single.go
index 324785a8..b7921837 100644
--- a/copy/single.go
+++ b/copy/single.go
@@ -819,8 +819,13 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
 				logrus.Debugf("Retrieved partial blob %v", srcInfo.Digest)
 				return true, updatedBlobInfoFromUpload(srcInfo, uploadedBlob), nil
 			}
-			logrus.Debugf("Failed to retrieve partial blob: %v", err)
-			return false, types.BlobInfo{}, nil
+			// On a "partial content not available" error, ignore it and retrieve the whole layer.
+			if chunkedToc.IsPartialContentNotAvailableError(err) {
+				logrus.Debugf("Failed to retrieve partial blob: %v", err)
+				err = nil
+			}
+			return false, types.BlobInfo{}, err
+
 		}()
 		if err != nil {
 			return types.BlobInfo{}, "", err

openshift-ci · 2024-09-30T08:08:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2024-09-30T14:23:36Z

LGTM
@nalind @Luap99 @mtrmac @mheon @vrothberg @saschagrunert PTAL

mtrmac

Starting from the c/image consumer, I’d prefer copy/single.go to not check for a specific c/storage error; c/image should define its own (private) error type for PutBlobPartial.

That, in turn, means that the c/storage code calling GetDiffer is going to already have a dependency on pkg/chunked; so we can put the error definition directly into pkg/chunked, and avoid the extra infrastructure to put it into chunked/internal+chunked/toc.

#2115 talks about adding a “require the GetDiffer code path to be used” option for fs-verity. We don’t need to do that right now, but how would that be added here?

It seems to me that we need a wrapping / unwrappable error mechanism (see underlying error in one of the comments), and then something like

func GetDiffer {
   …
   differ, err := getDifferInternal()
   if fallbackMarker, ok := errIsTheFallbackMarker(err); ok && fallbackDisabled {
      err = fallbackMarker.Unwrap() // no fallback, hard failure per configuration
   }
   …
}

with getDifferInternal directly, or some of its callees, adding the marker on relevant code paths.

pkg/chunked/storage_linux.go

mtrmac · 2024-09-30T15:44:00Z

pkg/chunked/storage_linux.go

-		if err != nil {
-			return nil, fmt.Errorf("parsing zstd:chunked TOC digest %q: %w", zstdChunkedTOCDigestString, err)
+		if err == nil {
+			return makeZstdChunkedDiffer(store, blobSize, zstdChunkedTOCDigest, annotations, iss, pullOptions)


#2115 (comment) talks about the case where a registry does not support ranged requests; that should be detected somewhere in makeZstdChunkedDiffer and turned into the “fall back” error. Similarly for the other differ.

pkg/chunked/internal/errors.go

pkg/chunked/storage_linux.go

cgwalters · 2024-09-30T19:05:36Z

Starting from the c/image consumer, I’d prefer copy/single.go to not check for a specific c/storage error; c/image should define its own (private) error type for PutBlobPartial.

One thing I don't quite understand is the circular relationship between the two projects...are we getting much out of having them be in separate git repositories even?

mtrmac · 2024-09-30T19:14:52Z

There are c/image users who don’t use c/storage (registry clients, mirroring tools). In fact that’s the primary point of that concern — a fair bit of the generic c/image logic should not be hard-coding c/storage specifics.
The relationship is not circular (apart from one callback from c/storage back, but I can remember only that one case where we want to invert control that way)
The test mechanisms and CI costs are very different (sometimes in bad ways, e.g. how most of c/image tests live in the Skopeo repo); but anyway, paying the registry testing costs for c/storage, or the c/storage filesystem/driver test matrix costs for c/image, is not immediately attractive.

The relationship is not set in stone, but then again there’s a substantial cost to integrate, and no obvious benefit to doing so.

giuseppe · 2024-10-01T08:46:40Z

#2115 talks about adding a “require the GetDiffer code path to be used” option for fs-verity. We don’t need to do that right now, but how would that be added here?

The flag will probably look like fsverity=disable|auto|require. The advantage with this PR is that /pkg/chunked can finally return an error that causes the pull to fail instead of falling back to the current pull mechanism.

pkg/chunked/storage_linux.go

mtrmac · 2024-10-02T13:30:17Z

pkg/chunked/storage_linux.go

-		if err != nil {
-			return nil, fmt.Errorf("parsing estargz TOC digest %q: %w", estargzTOCDigestString, err)
+		differ, err = makeZstdChunkedDiffer(store, blobSize, zstdChunkedTOCDigest, annotations, iss, pullOptions)
+		if err == nil {


If this fails because the registry does not support range requests, nothing marks the failure as eligible for fallback, AFAICS.

wouldn't it fail earlier when we request the TOC through a range request? So we should treat it as a failure here?

AFAICS we request the TOC inside this function, not earlier. Is that incorrect?

ah sorry, yes I got confused.

I've added the following snippet:

diff --git a/pkg/chunked/storage_linux.go b/pkg/chunked/storage_linux.go index ce8f8a2ce..26e8f1aba 100644 --- a/pkg/chunked/storage_linux.go +++ b/pkg/chunked/storage_linux.go @@ -196,6 +196,13 @@ func GetDiffer(ctx context.Context, store storage.Store, blobDigest digest.Diges } logrus.Debugf("Could not create differ for blob %q: %v", blobDigest, err) + + // If the error is a bad request to the server, then signal to the caller that it can try a different method. This can be done + // only when convert_images is disabled. + var badRequestErr ErrBadRequest + if errors.As(err, &badRequestErr) { + err = newErrFallbackToOrdinaryLayerDownload(err) + } return nil, err }

Avoid handling cases where the server doesn't support at least 64 ranges in a request, in order to prevent falling back to the traditional pull mechanism. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

define a new error type so that the caller can determine whether it is safe to ignore the error and retrieve the resource fully. Closes: containers#2115 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

mtrmac

LGTM. Thanks!

rhatdan · 2024-10-02T20:26:23Z

/lgtm

openshift-ci bot added the approved label Sep 30, 2024

giuseppe mentioned this pull request Sep 30, 2024

pkg/chunked/GetDiffer must differentiate between failures and errors that can be ignored #2115

Closed

giuseppe force-pushed the define-error-for-unsupported-partial-pulls branch from 1fe825a to 293985d Compare September 30, 2024 08:15

mtrmac reviewed Sep 30, 2024

View reviewed changes

giuseppe force-pushed the define-error-for-unsupported-partial-pulls branch from 293985d to 4b99d02 Compare October 1, 2024 08:12

giuseppe force-pushed the define-error-for-unsupported-partial-pulls branch from 4b99d02 to f095cb5 Compare October 1, 2024 08:47

mtrmac reviewed Oct 1, 2024

View reviewed changes

pkg/chunked/storage_linux.go Outdated Show resolved Hide resolved

pkg/chunked/storage_linux.go Show resolved Hide resolved

giuseppe force-pushed the define-error-for-unsupported-partial-pulls branch from f095cb5 to 8e426ac Compare October 2, 2024 08:25

mtrmac reviewed Oct 2, 2024

View reviewed changes

chunked: drop special handling for "< 64" ranges

3bbd19c

Avoid handling cases where the server doesn't support at least 64 ranges in a request, in order to prevent falling back to the traditional pull mechanism. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the define-error-for-unsupported-partial-pulls branch from 8e426ac to ea95255 Compare October 2, 2024 13:51

chunked: define error for partial pulls not available

ed13248

define a new error type so that the caller can determine whether it is safe to ignore the error and retrieve the resource fully. Closes: containers#2115 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the define-error-for-unsupported-partial-pulls branch from ea95255 to ed13248 Compare October 2, 2024 15:14

mtrmac reviewed Oct 2, 2024

View reviewed changes

openshift-ci bot assigned rhatdan Oct 2, 2024

openshift-ci bot added the lgtm label Oct 2, 2024

openshift-merge-bot bot merged commit 0eb3a02 into containers:main Oct 2, 2024
19 checks passed

giuseppe mentioned this pull request Oct 3, 2024

copy: attempt fallback to full retrieval only when possible containers/image#2589

Merged

This was referenced Oct 17, 2024

podman pull zstd:chunked-image, with vfs: not supported containers/podman#24308

Open

Update 330-corrupt-images.bats for composefs behavior change containers/podman#24314

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chunked: define error for partial pulls not available #2118

chunked: define error for partial pulls not available #2118

giuseppe commented Sep 30, 2024

openshift-ci bot commented Sep 30, 2024

rhatdan commented Sep 30, 2024

mtrmac left a comment

mtrmac Sep 30, 2024

cgwalters commented Sep 30, 2024

mtrmac commented Sep 30, 2024

giuseppe commented Oct 1, 2024

mtrmac Oct 2, 2024

giuseppe Oct 2, 2024

mtrmac Oct 2, 2024

giuseppe Oct 2, 2024

mtrmac left a comment

rhatdan commented Oct 2, 2024

chunked: define error for partial pulls not available #2118

chunked: define error for partial pulls not available #2118

Conversation

giuseppe commented Sep 30, 2024

openshift-ci bot commented Sep 30, 2024

rhatdan commented Sep 30, 2024

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Sep 30, 2024

Choose a reason for hiding this comment

cgwalters commented Sep 30, 2024

mtrmac commented Sep 30, 2024

giuseppe commented Oct 1, 2024

mtrmac Oct 2, 2024

Choose a reason for hiding this comment

giuseppe Oct 2, 2024

Choose a reason for hiding this comment

mtrmac Oct 2, 2024

Choose a reason for hiding this comment

giuseppe Oct 2, 2024

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

rhatdan commented Oct 2, 2024